-
Notifications
You must be signed in to change notification settings - Fork 15.9k
Integrate alembic check in ci
#49002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integrate alembic check in ci
#49002
Conversation
5513306 to
9a67d8b
Compare
|
Could you add a change to a model in a second commit on this PR (that we will shortly revert/undo) to test that this fails as expected please? |
|
Sure! I would do it later after this CI finished (make sure it could pass now) |
| ActionCommand( | ||
| name="check-models", | ||
| help="Check if there are model changes without a corresponding migration", | ||
| description="Check if the current models require new migrations to be generated", | ||
| func=lazy_load_command("airflow.cli.commands.db_command.check_models"), | ||
| args=(ARG_VERBOSE,), | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I'm not sure we want this in here though actually.
The airflow db is designed for end users/Airflow administrators to run, but the check command is only useful for Airflow devs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing, I would remove it from airflow db ~
9a67d8b to
f389818
Compare
f389818 to
b13a09c
Compare
b3e4358 to
31fa445
Compare
cc8b840 to
f09bb95
Compare
66cd694 to
4c3b055
Compare
4c3b055 to
9645890
Compare
2cc7087 to
ae67e5a
Compare
alembic checkalembic check in ci
9354811 to
01e5c34
Compare
|
|
|
Thanks for pointing out. I would modify my PR summary later. |
|
I've added a field named $ breeze shell "cd airflow-core/src/airflow && alembic check"
[2025-05-06T12:23:10.917+0000] {migration.py:207} INFO - Context impl SQLiteImpl.
[2025-05-06T12:23:10.919+0000] {migration.py:210} INFO - Will assume non-transactional DDL.
[2025-05-06T12:23:11.124+0000] {compare.py:400} INFO - Detected added column 'dag.test_field'
[2025-05-06T12:23:11.158+0000] {messaging.py:71} ERROR - New upgrade operations detected: [('add_column', None, 'dag', Column('test_field', String(length=2000), table=<dag>))]
FAILED: New upgrade operations detected: [('add_column', None, 'dag', Column('test_field', String(length=2000),
table=<dag>))]
Error 255 returnedbut $ pre-commit run check-model-changes
Check if model changes require DB migration..............................Passed |
wow. That's nice |
|
Did you check if we can have this as a test in utils/test_db.py |
a9cebfd to
d2a7d02
Compare
|
It works as well in my local computer. Let me test it on ci |
|
There's a bug in CI that might be causing all these to pass: https://github.com/apache/airflow/actions/runs/14898553456/job/41845929327?pr=49002#step:5:5931 |
|
Thanks a lots! |
acf3800 to
c4dcb81
Compare
|
Rebased and hope ci failed! |
|
So, the bug I fixed is unrelated. I think I understand what's happening. The DB is initialized from the ORM, and we try to run the file migration, too, by running downgrade and upgrade commands. In the end, the new field you added is already in the DB. When the In airflow 2, we could easily detect this new field because our tests initializes the database from the migration files instead of the ORM. |
|
Yes, I think that is unrelated. What you said is match with my local experiment. It would only successfully detect the error when I init with right ORM and then try add/delete something. I still try to figure how to deal with this without breaking the whole initialization process. |
|
Close now and would come back if I have better idea about this. |
Related Issue
closes #48998
cc: @ashb
Why
Since Alembic 1.9, a built-in command
alembic checkis available to checks if there are pending upgrade operations in the migration file, to integrating this check into our CI ensures that no model changes go unnoticed, preventing potential issues that could arise when applying migrations in production.How
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.